feat(ui): make stream framerate and bitrate configurable - #8
Open
alexander-clawthorne wants to merge 4 commits into
Open
alexander-clawthorne wants to merge 4 commits into
alexander-clawthorne wants to merge 4 commits into
Conversation
Adds TARGET FRAMERATE and MAX BITRATE rows to the settings screen so the GeForce NOW stream profile can be changed on device instead of being fixed at build time. Defaults are unchanged (30 fps, 6000 kbps), so behaviour on lower-end chips and wifi cards is preserved unless the user opts in. - persistent_settings: store frames_per_second and max_bitrate_kbps with save/parse entries; unknown keys are still skipped, so existing settings files load unchanged and stay readable by older builds - handheld_ui: expose both values to C through accessors that mirror the existing stream_width/stream_height pattern - geforce_now: read the framerate when the session client is created, and the bitrate ceiling at both request sites, replacing the fixed constant - settings_view: two new rows, framerate toggling 30/60 and bitrate cycling 4/6/8/12/16/20 Mbps Both values are negotiated when a stream starts, so a change applies to the next stream rather than the running one. The settings screen says so. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 6 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
selectStreamMode reset the stream rate to the hardcoded 30 fps and matched entitled resolutions against that constant, so a 60 fps setting never reached the session request. Track the configured rate separately and use it for both the fallback and the resolution match. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Only accept persisted framerate and bitrate values from the choices the settings view offers; anything else keeps the default. The choice lists now live in persistent_settings so the view and the parser share them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Clamp the configured ceiling to 4000-20000 kbps once and use it for both the NVST answer and the bitrate request, widening before the bps conversion so it cannot overflow. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Author
|
On-device result for Hardware and setup: RG35XX-H (H700), muOS. The line that was wrong before 9e42628: From the per-second stats (274 samples, 1s-275s):
The session ended at 275s because I saved and quit the game in CONTROL, so GeForce NOW sent a The bitrate ceiling isn't logged, so this run shows that 16000 was accepted and streamed fine. It doesn't show the ceiling being reached (the scene didn't need it). Log excerpt (session ID and ICE addresses redacted; stats sampled every 30s) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Following up on the Reddit thread where you said making these configurable made sense — the defaults are deliberately left alone so lower-end chips and wifi cards keep the behaviour they have now.
Based on
feat/geforce-now, so it targets that branch rather thanmaster.What does this change?
Adds two rows to the settings screen, TARGET FRAMERATE (30/60) and MAX BITRATE (4/6/8/12/16/20 Mbps), so the GeForce NOW stream profile can be changed on device instead of being fixed at build time.
persistent_settings.zig— storesframes_per_secondandmax_bitrate_kbpswith save/parse entries. Unknown keys are still skipped, so existingsettings.tsvfiles load unchanged and remain readable by older builds.handheld_ui.zig/handheld_ui.h— exposes both values to C through accessors mirroring the existingstream_width/stream_heightpattern.geforce_now/session_client.zig— reads the framerate when the client is created and uses it for stream-mode selection.geforce_now/webrtc_session.zig— reads the bitrate ceiling at both request sites (requestBitrateand the NVST SDP answer), replacing the fixedmaximum_bitrate_kbpsconstant, which is removed as it had no remaining references.settings_view.zig— the two rows, plus row spacing adjusted from 48px to 46px so six rows fit above the footer.Defaults are unchanged: 30 fps and 6000 kbps, matching the previous hard-coded values. Nothing changes for existing users unless they opt in. The Xbox path is untouched.
Two behaviours worth flagging for review:
sdp_protocol.zigderives the opening bid asmax(4000, ceiling/4). With ceilings at or below 16000 that stays 4000 as before; at 20000 it becomes 5000. Happy to cap the choices at 16000 if you'd rather the initial request never move.How did you test it?
Built with
tools/bootstrap.sh && tools/zig.sh build releaseon an x86_64 Linux host and deployed to an Anbernic RG35XX-H (H700) running muOS 2508.2.tools/zig.sh build test— passestools/zig.sh build fmt-check— passestools/zig.sh build release— passesThe 60 fps / 12 Mbps figures below come from a build carrying those values directly — the same profile the new settings select — captured with
GREENOVERCAST_DEBUG=1over several minutes of Control:60 frames/second,
cedar-h616hardware decode, no decode errors, no backpressure, no queue growth. Also verified settings persist across restarts and that a pre-existingsettings.tsvwithout the new keys still loads.I have not tested on Rockchip/MPP hardware or the Miyoo Flip — only the H700 above.
Review follow-up (cubic)
All four cubic findings were valid. They're fixed as separate commits, following the focused
fix(<scope>):pattern from #3:9e42628fix(gfn):selectStreamModeoverwrote the configured rate with the 30 fps constant and matched entitled resolutions against it. So in52900e5, choosing 60 in settings still requested 30. The 60 fps log above came from a build with the value hard-coded, which skips that path. That's why it didn't show up. New test:stream mode selection keeps the configured frame rate(fails when the fix is reverted).dbed9b6fix(ui): persisted framerate/bitrate are only accepted when they're one of the offered choices; otherwise the default stays. The choice lists moved intopersistent_settings.zigso the view and the parser share them. New test covers 0, 1 andu32max.bbf4d56fix(gfn): the bitrate ceiling is clamped to 4000-20000 kbps once, before both the NVST answer andrequestBitrate, and widened tou64before the bps conversion.Re-run after the fixes:
build test,build fmt-checkandbuild releaseall pass, andnm -D --undefined-only webrtc_stream | grep -c __aarch64_is 0. The fixed build is deployed to the RG35XX-H with settings at 60 fps / 16 Mbps. Confirmed on device: the session requested1024x768@60and averaged 60.15 fps over about 4.5 minutes with no decode errors. Details and a log excerpt are in this comment.AI disclosure
Per CONTRIBUTING: this was written with AI assistance (Claude). I reviewed the diff, built it, and verified the behaviour on hardware myself. Happy to adjust anything — naming, the bitrate choices, or the row ordering.
Summary by cubic
Makes GeForce NOW stream framerate and bitrate configurable from the settings screen, keeping the previous defaults of 30 FPS and 6 Mbps so existing behavior is unchanged.
Written for commit bbf4d56. Summary will update on new commits.